-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NEW: @W-17100389@: Add in feedback url to help text and log #1674
Conversation
stephen-carter-at-sf
commented
Nov 15, 2024
- Added give-feedback message and url to each command's help description
- Added give-feedback message and to end of the log file
- Removed draft markings of few remaining messages.
37eb1c8
to
c11ca62
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I have one little edit.
@@ -58,6 +59,7 @@ export class LogEventLogger implements LogEventListener { | |||
} | |||
|
|||
public stopListening(): void { | |||
this.logWriter.writeToLog('\n' + getMessage(BundleName.Shared, 'log.give-us-feedback')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Is it okay for this to be a raw string without a log-level, source, etc? I know that conceptually it doesn't have those things, but I'm just unsure of whether logging something that lacks them could interfere with displaying or filtering logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a good question. I had the same thought. I'm not sure if the goal is to make the logs machine readable or not. If so - then we'd need to adjust the formatting to be more deterministic since currently messages can already go to multiple lines and possibly corrupt the formatting.
I currently don't think we need the log to be machine readable - my guess is that it will always just be for human readability when diagnosing issues. So I think having a newline plus a plain string works here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough.